Skip to content
This repository was archived by the owner on Dec 9, 2024. It is now read-only.

Conversation

@tbarlow12
Copy link
Contributor

@tbarlow12 tbarlow12 commented Jun 21, 2019

Wrapper service for Azure Blob Storage functionality. To be used in update of deployment process for rollback capability.

Resolves [AB#361]

@mydiemho
Copy link
Contributor

@tbarlow12 please also add to description on what this PR is about

Copy link
Contributor

@PIC123 PIC123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small comment, but otherwise looks good!

* @param blobName Blob to delete
*/
public async deleteFile(containerName: string, blobName: string): Promise<void> {
const blockBlobUrl = await this.getBlockBlobURL(containerName, blobName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this one line like you have done with the deleteContainer method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question... Do they both get awaited?

Copy link
Contributor

@mydiemho mydiemho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be confused about how the blob sdk work, but there are undefined argument used

const containerURL = this.getContainerURL(containerName);
do {
const listBlobsResponse = await containerURL.listBlobFlatSegment(
Aborter.none,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this in a few places here. What does it do?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i believe marker is used for paging scenarios. It's undefined on the first iteration so the first page of data is returned.

do {
const listContainersResponse = await this.getServiceURL().listContainersSegment(
Aborter.none,
marker,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, marker is not defined first loop

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

Copy link
Contributor

@wbreza wbreza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few comments - otherwise looks good.

const containerURL = this.getContainerURL(containerName);
do {
const listBlobsResponse = await containerURL.listBlobFlatSegment(
Aborter.none,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i believe marker is used for paging scenarios. It's undefined on the first iteration so the first page of data is returned.

do {
const listContainersResponse = await this.getServiceURL().listContainersSegment(
Aborter.none,
marker,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

* @param ext - Extension of files to filter on when retrieving files
* from container
*/
public async listFiles(containerName: string, ext?: string): Promise<string[]> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please validate input arguments using Guard to make sure they satisfy the method requirements.

* Get ServiceURL object for Azure Blob Storage Account
*/
private getServiceURL(): ServiceURL {
const credential = this.credentials
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to use this.credentials directly vs split out into separate variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope :)

@tbarlow12 tbarlow12 merged commit 31cfbb8 into dev Jun 25, 2019
@tbarlow12 tbarlow12 deleted the tabarlow/blob-storage branch June 25, 2019 20:15
tbarlow12 added a commit that referenced this pull request Sep 13, 2019
Wrapper service for Azure Blob Storage functionality. To be used in update of deployment process for rollback capability.

Resolves [AB#361]
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants